Skip to content

Conversation

@abhinavdahiya
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added retest-not-required-docs-only size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2018
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This graph link depends on #100, although I don't think we need to block this PR on #100 landing. And this paragraph is sounding a lot like Make, Bazel, Terraform, etc. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this paragraph is sounding a lot like Make, Bazel, Terraform, etc. ;)

mini version of these :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asset is an map of all the assets...

This reads a bit awkwardly (one asset containing many?). Maybe just drop this last sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@wking wking Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a Merkle DAGs. Maybe our state structure should just be a DAG? That would build dependencies into the structure directly. I'm sure there are many off-the-shelf Merkle DAG libraries in Go (here's one), but rolling our own for this special case will probably not be hard either. Something like:

type State struct {
  Objects map[string]Asset // store for all the assets
  Root    string  // hash for the root asset
}

type Asset struct {
  Parents []string // hashes for our parents
  Name string
  Path string  // is this really important?  I'd have expected we can just use `Name` for this.
  Data []byte
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect []byte for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we decided that we gonna just throw our hands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's still an valid outcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the parent include everything but the final targets ?
I would expect some tree-list structure, besides how is this struct related to the Assets struct below?

I was thinking something like:

type State struct {
    Assets []Asset; // Final leaf assets
}
type Asset struct {
    Name string
    Parents []Asset
    Dirty bool
    Hash string
    Data []byte
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state object is an hashmap containing all the assets, not just the final. Each asset will use this object to fetch its dependencies.

@yifan-gu
Copy link
Contributor

yifan-gu commented Aug 4, 2018

@abhinavdahiya The description lgtm, but I have questions on the type definition.

@yifan-gu
Copy link
Contributor

yifan-gu commented Aug 7, 2018

Discussed with @abhinavdahiya , agree on the definition of the struct in this doc, just waiting on some fix of the naming.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Aug 7, 2018
Copy link
Contributor

@yifan-gu yifan-gu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit e44c706 into openshift:master Aug 8, 2018
@abhinavdahiya abhinavdahiya deleted the asset_generation branch August 8, 2018 20:11

// AssetState is the state of an Asset.
type AssetState struct {
Parents maps[string]string // hashes for our each parent.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this end up being a map? I think it should be a slice of hash strings; see here and here.

miyamotoh pushed a commit to miyamotoh/installer that referenced this pull request Feb 15, 2022
Attempt to remove warnings for "unused" TF variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. retest-not-required-docs-only size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants